Reviewed 56 of 56 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @buggmagnet)

ios/MullvadRustRuntime/PacketTunnelProvider+TCPConnection.swift line 100 at r1 (raw file):

) {
    guard let rawPostQuantumKeyReceiver else { return }
    let postQuantumKeyReceiver = Unmanaged<EphemeralPeerReceiver>.fromOpaque(rawPostQuantumKeyReceiver)

Nit: emphemeralKeyReceiver

ios/MullvadRustRuntime/PacketTunnelProvider+TCPConnection.swift line 110 at r1 (raw file):

    }

    // If there is a pre-shared key, an ephemeral peer was negotiated with Post Quantum options

Is it always true that the lack of a preshared key is daita? I guess it is for now, but what happens if we add new tech that uses the same infrastructure? I think EphemeralPeer is a fine collective term to use here, but I wonder if the implementation where the lack of a certain element guarantees one thing or the other.

ios/MullvadTypes/Protocols/EphemeralPeerReceiving.swift line 2 at r1 (raw file):

//
//  PostQuantumKeyReceiving.swift

Nit: EphemeralPeerReceiving

ios/MullvadVPN/TunnelManager/TunnelState.swift line 55 at r1 (raw file):

    case connecting(SelectedRelays?, isPostQuantum: Bool)

    // TODO: Add information here to support Daita ???

To be continued?

ios/MullvadVPN/TunnelManager/TunnelState.swift line 116 at r1 (raw file):

            "error state: \(blockedStateReason)"
        case let .negotiatingEphemeralPeer(tunnelRelays, _):
            // TODO: Handle Daita and PQ here

To be continued?

ios/MullvadVPN/TunnelManager/TunnelState+UI.swift line 52 at r1 (raw file):

            }

        // TODO: How to Handle Daita here ?

This part can be delegated to daita tickets related to UI. That should still be noted here I think.

ios/PacketTunnelCore/Actor/ObservedState+Extensions.swift line 19 at r1 (raw file):

        case .connecting:
            "Connecting"
        // TODO: Handle Daita here ?

I guess so. We should proably check the ObservedConnectionState is daita and/or post quantum is used and return an approriate name.

ios/PacketTunnelCore/Actor/PacketTunnelActor+PostQuantum.swift line 101 at r1 (raw file):

            // wireguard-go will only turn on daita for the entry peer,
            // so pass the daita configuration to the exit peer for consistency

What's meant here by passing config to exit for consistency? Can't see how the comment reflects the code.

ios/PacketTunnelCore/Actor/PacketTunnelActorCommand.swift line 76 at r1 (raw file):

            case .switchKey:
                return "switchKey"
            // TODO: Handle Daita here ???

Perhaps more cases are needed to handle this. We shouldn't just leave the Todo though.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @rablador)

ios/MullvadRustRuntime/PacketTunnelProvider+TCPConnection.swift line 110 at r1 (raw file):

I was debating whether I wanted to add a different FFI that specifies Daita only peers, but would increase a lot the cyclomatic complexity for virtually no benefits.

There cannot be a situation where we get an ephemeral peer that has an invalid preshared key, since the key is attached to the ephemeral peer when we request it.

ios/MullvadVPN/TunnelManager/TunnelState.swift line 55 at r1 (raw file):

Yes, there are a lot of TODO comments that will be fixed with working on the UI.

ios/PacketTunnelCore/Actor/PacketTunnelActor+PostQuantum.swift line 101 at r1 (raw file):

Oh that's a good catch, I had a previous design where I was passing the Daita configuration in the ConfigurationBuilder instead, and that's a relic of that past. I will delete the comment.

Reviewable status: 45 of 56 files reviewed, 7 unresolved discussions (waiting on @rablador)

ios/PacketTunnelCore/Actor/PacketTunnelActorCommand.swift line 76 at r1 (raw file):

I think we can safely remove this TODO here 🤔

Reviewed 10 of 11 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

ios/MullvadVPN/TunnelManager/TunnelState.swift line 55 at r1 (raw file):

I think we should address them as such then. Right now it looks like this PR still has TODOs rather than delegating them to another PR.

Reviewable status: 52 of 56 files reviewed, 4 unresolved discussions (waiting on @rablador)

ios/MullvadVPN/TunnelManager/TunnelState+UI.swift line 52 at r1 (raw file):

Done.

ios/PacketTunnelCore/Actor/ObservedState+Extensions.swift line 19 at r1 (raw file):

This is used for debugging purposes only, so it should be fine. I've removed the comment, we'll quickly see whether I was wrong to or not :D

Reviewable status: 52 of 56 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadRustRuntime/PacketTunnelProvider+TCPConnection.swift line 81 at r2 (raw file):

}

/// End sequence of a quantum-secure pre shared key exchange.

I believe this part should also be updated.

ios/MullvadRustRuntime/PacketTunnelProvider+TCPConnection.swift line 85 at r2 (raw file):

/// This FFI function is called by Rust when an ephemeral peer negotiation succeeded or failed.
/// When both the `rawPresharedKey` and the `rawEphemeralKey` are raw pointers to 32 bytes data arrays,
/// the quantum-secure key exchange is considered successful.

Key exchanging is taking place regardless of weather it's PQ or Daita. then I believe having quantum secure is forbidden.

Reviewable status: 52 of 56 files reviewed, 6 unresolved discussions (waiting on @mojganii and @rablador)

ios/MullvadRustRuntime/PacketTunnelProvider+TCPConnection.swift line 81 at r2 (raw file):

Good catch !

ios/MullvadRustRuntime/PacketTunnelProvider+TCPConnection.swift line 85 at r2 (raw file):

Yes, but that paragraph is still correct, since we still need to document the PQ only case

Reviewable status: 51 of 56 files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 42 at r5 (raw file):

            let token = runtime.packet_tunnel.tcp_connection.clone();

            let tokio_handle = match crate::mullvad_ios_runtime() {

Why move this function here from mod.rs?

Reviewable status: 51 of 56 files reviewed, 8 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 142 at r5 (raw file):

            match Self::ios_tcp_client(self.packet_tunnel.clone()).await {
                Ok(result) => result,

Unnecessary whitespace change.

Reviewed 1 of 11 files at r2.
Reviewable status: 51 of 56 files reviewed, 8 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

Reviewed 1 of 56 files at r1, 1 of 11 files at r2.
Reviewable status: 51 of 56 files reviewed, 8 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

Reviewable status: 51 of 56 files reviewed, 9 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 17 at r5 (raw file):

      "location" : "https://github.com/mullvad/wireguard-apple.git",
      "state" : {
        "revision" : "fb9a34f15e47b167866af3257a8dcc9901d9e7c1"

This is an outdated commit now.

Reviewable status: 51 of 56 files reviewed, 9 unresolved discussions (waiting on @mojganii, @pinkisemils, and @rablador)

mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 42 at r5 (raw file):

We used to pass the TokioHandle as part of run_post_quantum_psk_exchange which made clippy complain that we were passing too many parameters, I've moved creating (or getting) the runtime to just before we need it, It should still fail the same way as before if we can't get a runtime .

mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 142 at r5 (raw file):

We should agree on using a rust formatter then

Reviewed 3 of 4 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @mojganii, and @pinkisemils)

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @mojganii, and @pinkisemils)

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet and @mojganii)

mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 42 at r5 (raw file):

Fair. Interesting that this passed the CI last time. I'd prefer to keep this function free from that call, but if clippy is complaining, then we could also stuff all of the peer exchange args into a struct instead.

mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 142 at r5 (raw file):

We are using a rust formatter, and the whitespace is not being introduced by the formatter.

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mojganii and @pinkisemils)

mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 42 at r5 (raw file):

Yes, I'm not super happy about this either. I think if we want to revisit this code in the future, we should take the opportunity to refactor a bit to make it cleaner

mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 142 at r5 (raw file):

I've ran cargo fmt but it didn't change anything. I probably added some extra lines when I was refactoring. I'll remove the extra line.

ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 17 at r5 (raw file):

Done.

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @mojganii, and @pinkisemils)

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet and @mojganii)

mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 42 at r5 (raw file):

We can stuff the extra parameters in a struct today and move the instantiation of the tokio handle back to where it was before.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)

mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 42 at r5 (raw file):

Just to be clear, clippy lints are not there to be appeased, they are there to force us to think of a better design. I don't think moving a call between functions is the path to a better design in this case.

Reviewed 6 of 6 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)

Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)

Reviewed 1 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @mojganii)